Tests for Introduce config.FromEnv() #58
Conversation
| type Options map[string]zapcore.Level | ||
|
|
||
| // UnmarshalText implements encoding.TextUnmarshaler to allow Options to be parsed by env. | ||
| func (o *Options) UnmarshalText(text []byte) error { |
There was a problem hiding this comment.
It turns out the lib already supports []encoding.TextUnmarshaler, so why not map[T]encoding.TextUnmarshaler?
There was a problem hiding this comment.
Thanks for trying to solve this issue upstream. I have updated the PR to reflect this potentially upcoming change in the method's signature, https://github.com/Icinga/icinga-go-library/compare/7d7fd247cd9a69fddf46f237a6ae54ba475cb7e5..2ba2d8b19d9a9460f7f58cc2cd8d31561c03edf3.
However, I am against waiting for this PR to get merged and a new env release to get drafted. If this will be supported in the future, the custom encoding.TextUnmarshaler can just be removed on our end.
There was a problem hiding this comment.
As written over at your env PR, it works fine. Thanks again.
However, I would still say to merge it in the current state, eventually removing the encoding.TextUnmarshaler implementation later on. We have no "influence" on the upstream, resulting in blocking this PR, resulting in blocking your PR, resulting in having less tests (your PR would be nice to have in #60) and also resulting in blocking the icinga-go-library release.
By implementing the encoding.TextUnmarshaler, Options can be populated by environment variables from env.
|
I have just rebased this PR branch on top of the latest version of #41. @Al2Klimov: I am once again asking for your |
Al2Klimov
left a comment
Al2Klimov
left a comment
There was a problem hiding this comment.
- I'd prefer caarlos0/env#323, but it seems it is how it is.
For #41, as just requested by @Al2Klimov in #41 (comment).